Skip to content

add more tests for window::shift and handle boundary cases#386

Merged
alamb merged 3 commits intoapache:masterfrom
jimexist:shift-edge-cases
Jun 3, 2021
Merged

add more tests for window::shift and handle boundary cases#386
alamb merged 3 commits intoapache:masterfrom
jimexist:shift-edge-cases

Conversation

@jimexist
Copy link
Member

@jimexist jimexist commented May 31, 2021 •
edited
Loading

Which issue does this PR close?

Closes #387.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@codecov-commenter
Copy link

codecov-commenter commented May 31, 2021 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.64%. Comparing base (7ec3158) to head (daf1655).
Report is 2896 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #386      +/-   ##
==========================================
+ Coverage   82.62%   82.64%   +0.01%     
==========================================
  Files         162      162              
  Lines       44275    44310      +35     
==========================================
+ Hits        36582    36618      +36     
+ Misses       7693     7692       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jimexist jimexist force-pushed the shift-edge-cases branch from efa5781 to a5a70ff Compare May 31, 2021 02:38
@jimexist jimexist changed the title add more doc test for window::shift add more tests for window::shift and handle boundary cases May 31, 2021
@jimexist jimexist mentioned this pull request May 31, 2021
Merged
@nevi-me nevi-me requested a review from Dandandan June 2, 2021 01:06
nevi-me
nevi-me reviewed Jun 2, 2021
View reviewed changes
arrow/src/compute/kernels/window.rs Outdated
Copy link
Contributor

@nevi-me nevi-me Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as #388

Copy link
Member Author

@jimexist jimexist Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess it's not that easy - there's no easy way to clone array and likely suboptimal than just taking a slice here

nevi-me
nevi-me approved these changes Jun 2, 2021
View reviewed changes
Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment, LGTM otherwise

@jimexist jimexist force-pushed the shift-edge-cases branch from 64defcc to daf1655 Compare June 3, 2021 00:33
alamb
alamb reviewed Jun 3, 2021
View reviewed changes
arrow/src/compute/kernels/window.rs
/// let expected: Int32Array = vec![None, Some(4), None].into();
/// assert_eq!(res.as_ref(), &expected);
///
/// // shift array 0 element, although not recommended
Copy link
Contributor

@alamb alamb Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

alamb
alamb approved these changes Jun 3, 2021
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one looked good to me. Epic tests @jimexist

@alamb alamb merged commit 45acc62 into apache:master Jun 3, 2021
@alamb
Copy link
Contributor

alamb commented Jun 4, 2021

FWIW I could not cherry-pick this automatically to active_release (what will become release version 4.3):

@ip-10-0-0-124:~/Software/arrow-rs$ CHERRY_PICK_SHA=45acc62    ARROW_GITHUB_API_TOKEN=$ARROW_GITHUB_API_TOKEN CHECKOUT_ROOT=/tmp/arrow-rs  python3 dev/release/cherry-pick-pr.py
CHERRY_PICK_SHA=45acc62    ARROW_GITHUB_API_TOKEN=$ARROW_GITHUB_API_TOKEN CHECKOUT_ROOT=/tmp/arrow-rs  python3 dev/release/cherry-pick-pr.py
Using checkout in /tmp/arrow-rs
Creating cherry pick from 45acc62c to cherry_pick_45acc62c
remote: Total 0 (delta 0), reused 0 (delta 0), pack-reused 0        
From github.com:apache/arrow-rs
 * branch              master     -> FETCH_HEAD
From github.com:apache/arrow-rs
 * branch              active_release -> FETCH_HEAD
Switched to a new branch 'cherry_pick_45acc62c'
error: could not apply 45acc62c... add more tests for window::shift and handle boundary cases (#386)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
Command failed: ['git', 'cherry-pick', '45acc62']
With output:
--------------
b'Auto-merging arrow/src/compute/kernels/window.rs\nCONFLICT (content): Merge conflict in arrow/src/compute/kernels/window.rs\n'
--------------
Traceback (most recent call last):
  File "/Users/alamb/Software/arrow-rs/dev/release/cherry-pick-pr.py", line 153, in <module>
    make_cherry_pick()
  File "/Users/alamb/Software/arrow-rs/dev/release/cherry-pick-pr.py", line 116, in make_cherry_pick
    run_cmd(['git', 'cherry-pick', new_sha])
  File "/Users/alamb/Software/arrow-rs/dev/release/cherry-pick-pr.py", line 78, in run_cmd
    raise e
  File "/Users/alamb/Software/arrow-rs/dev/release/cherry-pick-pr.py", line 70, in run_cmd
    output = subprocess.check_output(cmd)
  File "/usr/local/Cellar/python@3.9/3.9.5/Frameworks/Python.framework/Versions/3.9/lib/python3.9/subprocess.py", line 424, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/usr/local/Cellar/python@3.9/3.9.5/Frameworks/Python.framework/Versions/3.9/lib/python3.9/subprocess.py", line 528, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['git', 'cherry-pick', '45acc62']' returned non-zero exit status 1.

If we want to include it we will have to backport it manually

alamb pushed a commit that referenced this pull request Jun 8, 2021
@jimexist @alamb
add more tests for window::shift and handle boundary cases (#386)
08fd10b 
* add more doc test for window::shift

* handle i64::MIN first

* use Ok(make_array(array.data_ref().clone()))
@alamb alamb mentioned this pull request Jun 8, 2021
Merged
@alamb
Copy link
Contributor

alamb commented Jun 8, 2021

Second chance back porting has worked #429 👍

alamb added a commit that referenced this pull request Jun 9, 2021
@alamb @jimexist
add more tests for window::shift and handle boundary cases (#386) (#429)
7f7d714 
* add more doc test for window::shift

* handle i64::MIN first

* use Ok(make_array(array.data_ref().clone()))

Co-authored-by: Jiayu Liu <Jimexist@users.noreply.github.com>
@jimexist jimexist deleted the shift-edge-cases branch June 10, 2021 02:02
@alamb alamb mentioned this pull request Jun 10, 2021
Merged
@zachschuermann zachschuermann mentioned this pull request Nov 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

@nevi-me nevi-me nevi-me approved these changes

@Dandandan Dandandan Awaiting requested review from Dandandan

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

window::shift shall properly handle boundary cases

4 participants

@jimexist @codecov-commenter @alamb @nevi-me